CLDR-16844 Show the Dashboard by default, and remember user preference #3934
Conversation
-New boolean dashWanted in cldrDashContext.mjs indicates whether the user wants the Dashboard open when appropriate -New boolean method cldrDashContext.shouldBeShown takes into account dashWanted, and whether a Special page (other than GENERAL_SPECIAL) is open -Add code to open the Dashboard (if shouldBeShown) where needed -Encapsulate the string, general, as cldrLoad.GENERAL_SPECIAL -Move message text to dash_needs_locale_and_coverage in cldrText.mjs, referenced in two places -Fix bug: effectiveName called without locale arg; console.error if it happens again -Remove confusing checkmark (vote.png) from General Info page; move style into GeneralInfo.vue -Fix call to getClient, await is required -Mark getClient as async to prevent VS Code wrongly warning that await is not needed -Remove dead code -Comments
-New boolean dashWanted in cldrDashContext.mjs indicates whether the user wants the Dashboard open when appropriate -New boolean method cldrDashContext.shouldBeShown takes into account dashWanted, and whether a Special page (other than GENERAL_SPECIAL) is open -Add code to open the Dashboard (if shouldBeShown) where needed -Encapsulate the string, general, as cldrLoad.GENERAL_SPECIAL -Move message text to dash_needs_locale_and_coverage in cldrText.mjs, referenced in two places -Fix bug: effectiveName called without locale arg; console.error if it happens again -Remove confusing checkmark (vote.png) from General Info page; move style into GeneralInfo.vue -Fix call to getClient, await is required -Mark getClient as async to prevent VS Code wrongly warning that await is not needed -Remove dead code -Comments
|
Hooray! The files in the branch are the same across the force-push. 😃 ~ Your Friendly Jira-GitHub PR Checker Bot |
| */ | ||
| function getClient() { | ||
| async function getClient() { | ||
| return new SwaggerClient({ |
There was a problem hiding this comment.
I thought I changed this code to cache the client. Maybe it's stuck in some PR.
I think the following is what we should be doing.
| return new SwaggerClient({ | |
| return await SwaggerClient({ |
There was a problem hiding this comment.
I'll test that locally...
There was a problem hiding this comment.
the async nor await is not needed in that case actually. just pass return SwaggerClient as a Promise back to the callers who await it.
There was a problem hiding this comment.
I'd say if the PR as is is working let's address this in a future one?
There was a problem hiding this comment.
Oh, well... I just tested with changing new to await, and it worked, so I made a commit...
There was a problem hiding this comment.
I'll test return SwaggerClient and make that change if it works
| @@ -24,7 +26,7 @@ const specialToComponentMap = { | |||
| add_user: AddUser, | |||
| auto_import: AutoImport, | |||
| downgraded: DowngradedVotes, | |||
| general: GeneralInfo, | |||
| general: GeneralInfo, // see cldrLoad.GENERAL_SPECIAL | |||
There was a problem hiding this comment.
we could integrate this better, but this is sufficient for now.
There was a problem hiding this comment.
Really all of these keys could benefit from encapsulation. I'm not sure what how/where, though... using cldrLoad.GENERAL_SPECIAL in place of general for the key isn't allowed in the initializer, but we could have initialization code like
specialToComponentMap[cldrLoad.GENERAL_SPECIAL] = GeneralInfo;
and likewise for the other keys...
| for (let i = 0; i < els.length; i++) { | ||
| els[i].style.display = "none"; | ||
| } |
There was a problem hiding this comment.
why this and not a reactive vue property?
There was a problem hiding this comment.
It was based on similar code in cldrDashContext.mjs. I can change it to a property...
-Revise getClient to use await SwaggerClient, per discussion in PR
|
Notice: the branch changed across the force-push!
~ Your Friendly Jira-GitHub PR Checker Bot |
-Revise getClient to use return SwaggerClient directly, per discussion in PR -Use a v-if property for visibility of Open Dashboard button, no need for general-open-dash class
-Delete extraneous exclamation point
-It was commented out, had uncommented-out for testing
| > | ||
| Open Dashboard | ||
| </button> | ||
|
|
||
| <!-- <cldr-overall-errors /> --> | ||
| <cldr-overall-errors /> |
There was a problem hiding this comment.
wait, did you mean to re-enable this? Why? I think we need to discuss first!
There was a problem hiding this comment.
I saw it too just after the commit, sorry about the noise! It was temporary for testing the call to cldrClient that motivated the whole tangent
-New boolean dashWanted in cldrDashContext.mjs indicates whether the user wants the Dashboard open when appropriate
-New boolean method cldrDashContext.shouldBeShown takes into account dashWanted, and whether a Special page (other than GENERAL_SPECIAL) is open
-Add code to open the Dashboard (if shouldBeShown) where needed
-Encapsulate the string, general, as cldrLoad.GENERAL_SPECIAL
-Move message text to dash_needs_locale_and_coverage in cldrText.mjs, referenced in two places
-Fix bug: effectiveName called without locale arg; console.error if it happens again
-Remove confusing checkmark (vote.png) from General Info page; move style into GeneralInfo.vue
-Fix call to getClient, await is required
-Mark getClient as async to prevent VS Code wrongly warning that await is not needed
-Remove dead code
-Comments
CLDR-16844
ALLOW_MANY_COMMITS=true